improvement: better 'strings' message to maintainers #17160

david-allison · 2024-09-29T17:08:12Z

We had a few cases where the process was not followed correctly

Improving the message should reduce the chance of this happening again

Fixes

Fixes Proposal: improve the 'Translation Sync' message #16980

Approach

Update the message

How Has This Been Tested?

Test results (on an issue, rather than a PR):

https://redirect.github.com/david-allison/Anki-Android/issues/42#issuecomment-2381427055

Learning (optional, can help others)

GitHub Actions are hard to test

"PR" URL was changed in Issue 15887:

Sync Translations workflow: run checks without manual open/close #15887

- https://github.com/ankidroid/Anki-Android/pulls/app%2Fgithub-actions
+ https://github.com/ankidroid/Anki-Android/pulls/mikehardy-machineaccount

Checklist

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

Arthur-Milchior

As someone who made the mistake, I fear, twice, I am, to be frank, not entirely sure it'll help.
If other people are like me, the issue is not only that the message is unclear.
The issue could be that it appears after the creation of the PR, and there may be a lot of discussion between the automated message and the time it's worth merging the PR.

Maybe having this appear after an approval would help. But I doubt it. Because, most importantly, that seems to be noise. Same as the "This is your first PR, welcome to ankidroid" and, worst offender "this is your monthly reminder you can use open collective".
I'm really not used to see messages that are actually relevant here.

On the other paw, now that I know that string must be synced, I don't expect I'll do the message again.

.github/workflows/label.yml

Arthur-Milchior · 2024-09-29T18:03:13Z

.github/workflows/label.yml

+            > **Maintainers**: This PR contains https://github.com/ankidroid/Anki-Android/labels/Strings changes
+
+            1. [Sync Translations](https://github.com/ankidroid/Anki-Android/actions/workflows/sync_translations.yml) before merging this PR and wait for the action to complete
+            2. Find the [auto-generated PR](https://github.com/ankidroid/Anki-Android/pulls/app%2Fgithub-actions) and open and close the PR to perform a lint check. Review and merge the PR to sync all user-submitted translations


If you want to be very specific in the message, I think a point that is simply about the review should be added.
It was very unclear to me what kind of review I could do given that I don't speak most languages.

Review that translations seems legit. Most troll can be easily detected (not the same character set as the remaining of the field, insults written in English...).
3'. If there are errors do [what should be done. Or put a link to there]

Merge the string sync PR.

....

Pushback: "How to do a code review" is out of scope here.

This should be a 'what' to do, rather than a 'how' to do it

mikehardy · 2024-09-29T18:23:53Z

@Arthur-Milchior only replying to note I see this:

worst offender "this is your monthly reminder you can use open collective".

...happy to do it some other way but I can't think of a way to reliably inform people at the moment?

Arthur-Milchior · 2024-09-29T18:43:09Z

@mikehardy No, I've no better idea.
Ideally, I'd just block this message, so that it stops popping in my notification for every PR in the ankidroid repo. But as far as I can see, I can't just block one user/bot. (Unless I block them for the whole repo, which I obviously won't do)

My point is only that, once a month (on average) there is a bunch of notification that I can just ignore. I just select them and mark the notification as "read".

I can't imagine any way to improve things here. At least not at AnkiDroid level, it would require a change in Github behaviour.

To take a step back, my main point was not about any specific message. Just about the fact that, I kind of learned that the bot's message are very repetitive, and so it's hard to pay attention to them.
But that's great. It means that any newcomer could read it once or twice. And any oldcomer could just state "great, this is time to ask to be paid!"

david-allison · 2024-09-30T00:58:29Z

As someone who made the mistake, I fear, twice, I am, to be frank, not entirely sure it'll help.
If other people are like me, the issue is not only that the message is unclear.
The issue could be that it appears after the creation of the PR, and there may be a lot of discussion between the automated message and the time it's worth merging the PR.

I'm hoping that using eye-catching admonitions reduces the chance of bad merges from happening.

There's a number of further automation steps which we can discuss & take which could block PR merges with strings, but I feel that this effort would be better spent on streamlining the translation sync process.

Specifically:

~~Remove the need to open + close PRs:~~ Sync Translations workflow: run checks without manual open/close #15887
~~Better linkage from lint errors to Crowdin (heck, it's 5 minutes, I'll do this now):~~ improvement(lint): 'TranslationTypo' - link to Crowdin #17164

mikehardy · 2024-09-30T01:10:39Z

Remove the need to open + close PRs

That one in particular is galling to me. It has to be something about the workflow syntax and what events trigger it. I can't believe a push on the branch doesn't do it ? maybe it's a different event? I haven't investigated but what a silly thing to have to do, and personally frustrating because...generally I'm good with workflows but that one resisted my previous attempts

david-allison · 2024-09-30T01:51:39Z

See: Sync Translations workflow: run checks without manual open/close #15887

GitHub has restrictions on recursive workflow runs, and recommends a machine account to resolve it

lukstbit

I was already ok with the changes from the issue's discussion.

david-allison · 2024-09-30T15:10:24Z

No need to open & close the PR. Fixed today

Sync Translations workflow: run checks without manual open/close #15887

david-allison · 2024-09-30T15:31:00Z

Updated to:

Important

Maintainers: This PR contains Strings changes

Sync Translations before merging this PR and wait for the action to complete
Review and merge the auto-generated PR in order to sync all user-submitted translations
Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

david-allison · 2024-09-30T15:34:48Z

Updated the URL to:

https://github.com/ankidroid/Anki-Android/pulls/mikehardy-machineaccount

We had a few times where the process was not followed correctly Improving the message should reduce the chance of this happening again Additional links further streamline the process for those who do not need the knowledge transfer Test results: https://redirect.github.com/david-allison/Anki-Android/issues/42#issuecomment-2381427055 Fixes 16980 Note: "PR" URL was changed in Issue 15887 ```diff - https://github.com/ankidroid/Anki-Android/pulls/app%2Fgithub-actions + https://github.com/ankidroid/Anki-Android/pulls/mikehardy-machineaccount ```

mikehardy

tidy set of quality-of-life improvements in our automation, considering all the related changes together. Very nice

Arthur-Milchior approved these changes Sep 29, 2024

View reviewed changes

david-allison changed the title ~~improvement: improve 'strings' message to maintainers~~ improvement: 'strings' message to maintainers Sep 30, 2024

david-allison changed the title ~~improvement: 'strings' message to maintainers~~ improvement: better 'strings' message to maintainers Sep 30, 2024

david-allison force-pushed the 16980 branch from 3ad5ce8 to 555c507 Compare September 30, 2024 00:53

david-allison mentioned this pull request Sep 30, 2024

improvement(lint): 'TranslationTypo' - link to Crowdin #17164

Merged

5 tasks

lukstbit approved these changes Sep 30, 2024

View reviewed changes

lukstbit added the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Sep 30, 2024

david-allison added the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2024

david-allison force-pushed the 16980 branch from 555c507 to 5d78ab7 Compare September 30, 2024 15:30

david-allison removed the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2024

david-allison force-pushed the 16980 branch from 5d78ab7 to a67afcb Compare September 30, 2024 15:34

david-allison added the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2024

david-allison force-pushed the 16980 branch from a67afcb to fddf0b8 Compare September 30, 2024 15:37

david-allison removed the Needs Author Reply Waiting for a reply from the original author label Sep 30, 2024

mikehardy approved these changes Sep 30, 2024

View reviewed changes

mikehardy added this pull request to the merge queue Sep 30, 2024

Merged via the queue into ankidroid:main with commit 715fa92 Sep 30, 2024
9 checks passed

github-actions bot added this to the 2.19 release milestone Sep 30, 2024

github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Sep 30, 2024

david-allison deleted the 16980 branch September 30, 2024 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: better 'strings' message to maintainers #17160

improvement: better 'strings' message to maintainers #17160

david-allison commented Sep 29, 2024 •

edited

Loading

Arthur-Milchior left a comment

Arthur-Milchior Sep 29, 2024

david-allison Sep 30, 2024

mikehardy commented Sep 29, 2024

Arthur-Milchior commented Sep 29, 2024

david-allison commented Sep 30, 2024 •

edited

Loading

mikehardy commented Sep 30, 2024

david-allison commented Sep 30, 2024 •

edited

Loading

lukstbit left a comment •

edited

Loading

david-allison commented Sep 30, 2024 •

edited

Loading

david-allison commented Sep 30, 2024

david-allison commented Sep 30, 2024

mikehardy left a comment

improvement: better 'strings' message to maintainers #17160

improvement: better 'strings' message to maintainers #17160

Conversation

david-allison commented Sep 29, 2024 • edited Loading

Fixes

Approach

How Has This Been Tested?

Learning (optional, can help others)

Checklist

Arthur-Milchior left a comment

Choose a reason for hiding this comment

Arthur-Milchior Sep 29, 2024

Choose a reason for hiding this comment

david-allison Sep 30, 2024

Choose a reason for hiding this comment

mikehardy commented Sep 29, 2024

Arthur-Milchior commented Sep 29, 2024

david-allison commented Sep 30, 2024 • edited Loading

mikehardy commented Sep 30, 2024

david-allison commented Sep 30, 2024 • edited Loading

lukstbit left a comment • edited Loading

Choose a reason for hiding this comment

david-allison commented Sep 30, 2024 • edited Loading

david-allison commented Sep 30, 2024

david-allison commented Sep 30, 2024

mikehardy left a comment

Choose a reason for hiding this comment

david-allison commented Sep 29, 2024 •

edited

Loading

david-allison commented Sep 30, 2024 •

edited

Loading

david-allison commented Sep 30, 2024 •

edited

Loading

lukstbit left a comment •

edited

Loading

david-allison commented Sep 30, 2024 •

edited

Loading